adds ADR to expose tuning parameters - #978
Conversation
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
I support option B. I think strict typing is critical here. As I understand it, option C would see any JSON object and validate it based on data type alone. Even if it's probably fine in practice, delegating validation elsewhere in the codebase feels risky to me, and not worth the perceived benefits.
|
|
||
| This creates a mismatch for downstream clients that validate against introspection-derived schema artifacts, including graphql-client. Those clients fail validation because the schema they consume does not include arguments that our internal callers (timdex-ui in particular) need to send. | ||
|
|
||
| Our GraphQL API is public, but exposing advanced tuning arguments is acceptable, even if most external users do not need them. We can state clearly they are likely not useful to most users in the documentation exposed by the Introspection queries. |
There was a problem hiding this comment.
I think it would be useful to state clearly why it's acceptable to expose advanced tuning arguments. I assume the worst case scenario is that users who fiddle with the settings might get weird results, which feels low risk.
There was a problem hiding this comment.
Great point. Yes, the risk for all of our current tuning parameters is excluding results that would have been useful or returning way more irrelevant results after the main good results. I'll update to clarify explicitly why this is low risk as you suggested.
|
|
||
| Initial recognized keys: | ||
|
|
||
| - mustBoostThreshold (Float, 0.0 to 1.0) |
There was a problem hiding this comment.
Probably not worth mentioning in this doc, as it's an implementation detail, but I don't think we've done this kind of input limitation in TIMDEX before. I'd imagine it would be pretty straightforward.
|
|
||
| Tips: | ||
|
|
||
| - Add test coverage for accepted and invalid tuning values. |
There was a problem hiding this comment.
Ignore my comment above about input validation. I think this addresses it, since test coverage would imply existing validation code.
3 participants
No description provided.